Skip to content

feat: add pypi attestation discovery #1067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

benmss
Copy link
Member

@benmss benmss commented Apr 24, 2025

Summary

This PR adds discovery of PyPI attestation. URLs to these attestation files are sought via the deps.dev API.

Description of changes

  • DepsDevRepoFinder was updated to use the DepsDevService, ensuring consistent and easily configurable use of the API
  • Tests were added for DepsDevRepoFinder functions (they were not added previously), including for the functions that PyPI attestation discovery relies upon.
  • PyPI attestations do not have a predicate. The pypi-attestation is used to extract information from the attestation certificate. This information is coerced into a predicate for use elsewhere within Macaron.
  • Addition of an integration test case using the ultralytics Python library as its target.

Related issues

Closes #947

@benmss benmss self-assigned this Apr 24, 2025
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Apr 24, 2025
@benmss benmss force-pushed the 947-discover-pypi-attestation branch from b97a9cb to a362d7c Compare April 24, 2025 04:28
@benmss benmss force-pushed the 947-discover-pypi-attestation branch from 2df212b to 6d7cf95 Compare April 24, 2025 06:31
@benmss benmss marked this pull request as ready for review April 24, 2025 13:00
@benmss benmss requested review from behnazh-w and tromai as code owners April 24, 2025 13:00
benmss added 2 commits May 2, 2025 14:08
Signed-off-by: Ben Selwyn-Smith <[email protected]>
Signed-off-by: Ben Selwyn-Smith <[email protected]>
@@ -83,6 +103,13 @@ def _load_provenance_file_content(
# Some provenances, such as Witness may not include the DSSE envelope `dsseEnvelope`
# property but contain its value directly.
provenance_payload = provenance.get("payload", None)
if not provenance_payload:
# GitHub Attestation.
# TODO Check if old method (above) actually works.
Copy link
Member

@tromai tromai May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could a doc string be added here - to explain the reason why there is a change in handling Github attestation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps @behnazh-w could weigh in on this?

raise ValueError(f"Missing certificate value: {name}")

# Values are DER encoded UTF-8 strings. Removing the first two bytes seems to be sufficient.
value: str = certificate_claims[name][2:].decode("UTF-8")
Copy link
Member

@tromai tromai May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we perform this operation in the first loop ?

for extension in certificate.extensions:
    ...

I think the purpose of this loop is for validating certificate_claims content, so in my opinion, it's best if we don't do extra mutation to certificate_claims.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have combined the loops.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. It looks okay to me. Just a small observation that the logic for checking whether certificate_claims contains enough claim names (e.g. source_repo, source_digest, etc.) has been removed in the latest commit. Just want to make sure that this is intended.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I'll add it back in.

Copy link
Member

@tromai tromai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have finished my round of review. Thank you.

benmss added 4 commits May 7, 2025 14:10
Signed-off-by: Ben Selwyn-Smith <[email protected]>
Signed-off-by: Ben Selwyn-Smith <[email protected]>
Signed-off-by: Ben Selwyn-Smith <[email protected]>
benmss added 3 commits May 8, 2025 23:13
Signed-off-by: Ben Selwyn-Smith <[email protected]>
Signed-off-by: Ben Selwyn-Smith <[email protected]>
Signed-off-by: Ben Selwyn-Smith <[email protected]>
Copy link
Member

@tromai tromai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for addressing the feedbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Obtain PyPI Publish Attestation
2 participants